adding support for switches: Force,AllowClobber,SkipPublisherCheck #337

mrhockeymonkey · 2018-02-09T21:04:50Z

This is to resolve issue #327

Not very much interesting, simply adding support for the switches used in PackageManagement.

msftclas · 2018-02-09T21:05:01Z

All CLA requirements met.

jianyunt · 2018-03-01T01:43:06Z

src/Microsoft.PackageManagement.DscResources/MSFT_PackageManagement/MSFT_PackageManagement.psm1

+
+        [Parameter()]
+        [Switch]
+        $AllowClobber,


First thank you very much for your PR. Here are my comments:

any reasons you want to explicitly call out these parameters instead of using the existing $AdditionalParameters to pass them in?

this DSC resource is a kind of generic for all OneGet providers. My concern is that not all the OneGet providers support force, AllowClobber, SkipPublisherCheck. If people choose to use other providers and this dsc resource, their scenario will be broken, right?

I did first try specifying these switches using $AdditionalParameters like this :

AdditionalParameters = @{ Force = $true }

However this threw errors when trying to convert the value to a switch. I believe this is because $PSBoundParameters is a key value store but switches don't take a value they are a bit special. This issue is also reported by others (#327)

this is a good design in principle. But it also means that if other providers have switches we will not be able to use them. I've been trying to find a better way to pass switches (something like $AdditionalSwitches) in but without much success so far

@brywang-msft any ideas in the PM DSC module regarding AdditionalParameters ?

I've just learnt that -AllowClobber doesn't exist for cmdlets in PackageManagement in WMF 5.0, looks like it is only present in 5.1 which means this approach I have wont work.

I'm happy if you want to close this.

Correct AllowClobber does not exist in PackageManagement. It was added in PowerShellGet - a PackageManagement provider. Thanks @mrhockeymonkey.

mrhockeymonkey · 2018-05-08T19:07:44Z

Apologies for reopening this. I think there is a bit of confusion over where updates to the package management DSC resources should live. We decided in this PR above that the DSC Resource should be generic which led me to open a PR (PowerShell/PackageManagementProviderResource#40).

However ive just seen that someone else has also done so (PowerShell/PackageManagementProviderResource#39) to which you have directed him back to this repo?

It seems to me that if the PackageManagementProviderResource repo is now deprecated we should pull the PSModule DSCResource into this repo? would you agree? I'm happy to add that to this PR if so along with the original fix.

jianyunt · 2018-05-12T01:16:02Z

@mrhockeymonkey Yes PackageManagementProviderResource repo is deprecated. Use this repo going forward. Sorry for keeping asking you to submit the PRs back and forth. Our thinking was to ask users to use the generic DSC MSFT_PackageManagement. That's why we did not port the MSFT_PSModule DSC resource. However it looks like folks are already using MSFT_PSModule. I think we should consider porting the MSFT_PSModule from PackageManagementProviderResource to PackageManagement here too.

Yes it would be great if you can

Port the MSFT_PSModule DSC here and add your fixes to the MSFT_PSModule. Remove the changes in MSFT_PackageManagement.
Could you please integrate the changes from @brwilkinson Added the AllowClobber parameter to the PSModule resource PowerShell/PackageManagementProviderResource#39 too?
please update mof, mfl too
@brwilkinson, did you mention there is a bug in $AdditionalParameters in MSFT_PackageManagement DSC https://github.com/PowerShell/PackageManagementProviderResource/blob/master/DSCResources/MSFT_PackageManagement/MSFT_PackageManagement.psm1#L350? We should use $AdditionalParameters.GetEnumerator()) instead?
If so, I recommand whether we can fix all together in this PR?

cc @JKeithB @edyoung

brwilkinson · 2018-05-12T05:22:22Z

@jianyunt There was actually additional work needed there. Which is why I went back to updating the original changes that I had made on PSModule. So the it would need further review before making any changes/decisions on the PackageManagement resource.

mrhockeymonkey · 2018-05-12T12:01:51Z

@jianyunt sounds good to me. would it make sense for me to start over with a fresh PR or do you want to keep this one for open for the discussion?

UPDATE: Upon starting this I see that the PSModule Resource uses the PowerShellGet module.

$moduleFound |  PowerShellGet\Install-Module -ErrorVariable ev

So this would introduce a cross module dependency. Not sure how i missed this the first time but on seeing this i think this repo is the best place for PSModule resource to live

https://github.com/PowerShell/PowerShellGet

Didn't realize these were separate module because of the similar naming i guess. Do you agree? This keeps the packagemanagement module generic as you had wished.

jianyunt · 2018-05-14T18:11:10Z

@brwilkinson thanks for you update and digging into it further. No worries we can migrate the psmodule dsc here first.

jianyunt · 2018-05-14T18:18:58Z

@mrhockeymonkey, yes PSModule DSC uses PowerShellGet. we have an few options:

All of the bug fixes in PSModule DSC goes to the https://github.com/PowerShell/PackageManagementProviderResource repo although it's deprecated. We can consider publishing hotfixes only.
PSModule DSC goes to PowerShellGet? CC @bmanikm
Users have to migrate their dsc to the generic PackageManagement DSC.
Move it here. if so PackageManagement module and PowerShellGet will have cross dependency. Also PackageManagement do not seem to be cohesive.

mrhockeymonkey · 2018-06-15T08:16:22Z

I would probably favor option 2 as it seems the most sensible. Sorry i haven't replied, i was waiting for a response from @bmanikm before I do go ahead in case I end up confusing matters more.

bmanikm · 2018-06-15T19:01:11Z

I agree PSModule DSC Resource should be part of the PowerShellGet module. Similarly, PSScript DSC Resource is required for the script items.

jianyunt · 2018-06-15T19:34:52Z

Cool! Thanks @bmanikm.
@mrhockeymonkey, you may consider submitting a PR to https://github.com/PowerShell/PowerShellGet. Please add your fixes to the MSFT_PSModule in your PR. Thank you for your help!

alerickson · 2018-11-29T22:48:38Z

Closing due to PR being moved to https://github.com/PowerShell/PowerShellGet/pull/347

jrykowski-huron · 2020-10-21T01:15:18Z

Closing due to PR being moved to https://github.com/PowerShell/PowerShellGet/pull/347

Getting a 404 on that pull 347 link.

This doesn't appear to have been merged to the master branch yet? It's a blocking issue for rollout of DSC for our work.

adding support for switches: Force,AllowClobber,SkipPublisherCheck

62dd3b1

mrhockeymonkey mentioned this pull request Feb 9, 2018

Can't set AllowClobber on PackageManagement DSC via AdditionalParameters property #327

Open

jianyunt reviewed Mar 1, 2018

View reviewed changes

jianyunt requested a review from brywang-msft March 1, 2018 01:43

jianyunt closed this Mar 20, 2018

mrhockeymonkey mentioned this pull request Apr 12, 2018

adding support for install-module switches PowerShell/PackageManagementProviderResource#40

Closed

mrhockeymonkey deleted the DSCResourceSupportForSwitches branch May 8, 2018 13:08

mrhockeymonkey restored the DSCResourceSupportForSwitches branch May 8, 2018 19:08

jianyunt reopened this May 12, 2018

jianyunt added the Discussion label May 12, 2018

mrhockeymonkey mentioned this pull request Oct 10, 2018

Adding support for PowerShellGet switches PowerShell/PowerShellGetv2#347

Merged

alerickson closed this Nov 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding support for switches: Force,AllowClobber,SkipPublisherCheck #337

adding support for switches: Force,AllowClobber,SkipPublisherCheck #337

mrhockeymonkey commented Feb 9, 2018 •

edited

Loading

msftclas commented Feb 9, 2018 •

edited

Loading

jianyunt Mar 1, 2018 •

edited

Loading

mrhockeymonkey Mar 6, 2018

jianyunt Mar 8, 2018

mrhockeymonkey Mar 20, 2018

jianyunt Mar 20, 2018

mrhockeymonkey commented May 8, 2018

jianyunt commented May 12, 2018

brwilkinson commented May 12, 2018

mrhockeymonkey commented May 12, 2018 •

edited

Loading

jianyunt commented May 14, 2018

jianyunt commented May 14, 2018 •

edited

Loading

mrhockeymonkey commented Jun 15, 2018

bmanikm commented Jun 15, 2018

jianyunt commented Jun 15, 2018

alerickson commented Nov 29, 2018

jrykowski-huron commented Oct 21, 2020

adding support for switches: Force,AllowClobber,SkipPublisherCheck #337

adding support for switches: Force,AllowClobber,SkipPublisherCheck #337

Conversation

mrhockeymonkey commented Feb 9, 2018 • edited Loading

msftclas commented Feb 9, 2018 • edited Loading

jianyunt Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

mrhockeymonkey Mar 6, 2018

Choose a reason for hiding this comment

jianyunt Mar 8, 2018

Choose a reason for hiding this comment

mrhockeymonkey Mar 20, 2018

Choose a reason for hiding this comment

jianyunt Mar 20, 2018

Choose a reason for hiding this comment

mrhockeymonkey commented May 8, 2018

jianyunt commented May 12, 2018

brwilkinson commented May 12, 2018

mrhockeymonkey commented May 12, 2018 • edited Loading

jianyunt commented May 14, 2018

jianyunt commented May 14, 2018 • edited Loading

mrhockeymonkey commented Jun 15, 2018

bmanikm commented Jun 15, 2018

jianyunt commented Jun 15, 2018

alerickson commented Nov 29, 2018

jrykowski-huron commented Oct 21, 2020

mrhockeymonkey commented Feb 9, 2018 •

edited

Loading

msftclas commented Feb 9, 2018 •

edited

Loading

jianyunt Mar 1, 2018 •

edited

Loading

mrhockeymonkey commented May 12, 2018 •

edited

Loading

jianyunt commented May 14, 2018 •

edited

Loading